feat: enabled contract upgrades and used namespaced storage #187
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
+ Coverage 96.83% 96.97% +0.14%
==========================================
Files 11 11
Lines 537 562 +25
==========================================
+ Hits 520 545 +25
Misses 17 17 ☔ View full report in Codecov by Sentry. |
srdtrk
changed the title
There was a problem hiding this comment.
Great job!! We're getting closer to having the full setup running 🚀
Left a couple of questions, but I also think updating the benchmarks might be needed.
In addition, make sure that any remaining implementation work (such as doing this for the light client, and considering how to deal with IBCStore) is documented in the Epic.
| /// @param escrow The escrow contract | ||
| /// @param ibcDenomContracts Mapping of non-native denoms to their respective IBCERC20 contracts | ||
| /// @custom:storage-location erc7201:ibc.storage.ICS20Transfer | ||
| struct ICS20TransferStorage { |
There was a problem hiding this comment.
I'm curious if this has any penalty in gas cost? Would this end up fetching more data than we need (+ use more memory) whenever we need only one of them?
I hope that the fact that it is set as a storage type helps, but still want to make sure we are not getting punished too hard for this :)
Have you tried to run the benchmarks to see if anything has changed significantly?
There was a problem hiding this comment.
Based on my understanding of solidity, this doesn't fetch more data/memory than we need, but we might be consuming more gas since we are making some extra function calls.
I updated the benchmarks and there is an increase in gas usage, but this might be due to the addition of the proxy contract, and fallback logic as well. The gas increase is not substantial in the aggregated scenario.
| @@ -305,4 +340,12 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| /// @notice Returns the storage of the ICS26Router contract | |||
| function _getICS26RouterStorage() private pure returns (ICS26RouterStorage storage $) { | |||
There was a problem hiding this comment.
Ditto for this and all the other ones where we have a struct as storage
| @@ -45,7 +47,31 @@ abstract contract FixtureTest is Test { | |||
| } | |||
|
|
|||
| function setUp() public { | |||
| ics26Router = new ICS26Router(address(this)); | |||
| // ============ Step 1: Deploy the logic contracts ============== | |||
There was a problem hiding this comment.
It looks like we do this a lot, can we refactor it out somehow? Not a big deal if it is not easy, but would be nice to have a single place where we do instantiation.
There was a problem hiding this comment.
I think this is not easy at the moment. The ideal scenario would be to use the deploy script but I'm not sure if it could be done. Happy to review any PR that does this
|
I updated the benchmarks, I think the light client itself doesn't need upgradability or recovery since this can be done by migrating ICSCore. I'm not sure if IBCStore needs upgradability either. I think I can add a testing issue in the epic for light client recovery (through ICSCore) |
Let's go through the use cases, but I can imagine that it could make sense for light client upgradability to be independent of ICSCore. If multiple light clients from different chains are using the same ICSCore, independent upgradability of light clients might be useful. |
Description
closes: #188, #189
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.